-
Notifications
You must be signed in to change notification settings - Fork 56
Hw2 Yury Popov #14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Hw2 Yury Popov #14
Conversation
Add multiplication
Divide function code
add substract function
Add summation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
В целом отлично! Хорошие сообщения коммитов. Отдельное спасибо за PEP8! Очень хорошо, что знаете про if __name__ == '__main__':
, докстринги и аннотацию, но про это еще будем говорить :)
То же касается и исключений, хотя с ними тут немного косяк... но ничего, это все будет дальше :)
@@ -0,0 +1,64 @@ | |||
def substract(a, b): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Тут и дальше. Названия a
и b
абсолютно не информативны :)
Лучше хотя бы num1
и num2
.
|
||
def divide(a, b): | ||
if b == 0: | ||
return "Error: division by zero" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Идея неплохая, но 2 момента:
- Если нам придет 0, то функция вернет строку. Это явно не то поведение, которое мы ожидаем :)
В этом случае уж лучше использоватьraise
, но об этом поговорим в следующем семестре :) - В целом-то можно даже и не обрабатывать :)
Ну то есть вот пользователь ввел 0 --> программа упала с ZeroDivisionError. Ожидаемое поведение :)
У нас тут нет какого-то варианта, чтоб при 0 мы бы все-равно хотели что-то да рассчитать.
|
||
|
||
def multiplication(a, b): | ||
answer = a * b |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: можно сразу return a * b
''' | ||
math_sign, operator_position = False, False | ||
|
||
for char in range(len(keyboard_input)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
тут явно не char
:)
вот если for char in keyboard_input
, тогда да... Здесь же скорее индекс ))
То есть либо char_idx
, либо в целом-то можно даже i
:)
А вообще можно было бы вот так:
for i, char in enumerate(keyboard_input):
if char in list_of_operators:
math_sign = char
operator_position = i
break
are not in list or numbers cannot transform into floats, | ||
func returns error message. | ||
''' | ||
math_sign, operator_position = False, False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Почему False
?
У вас же в math_sign
будет сидеть строка, а в operator_position
-- число ))
operator_position = char | ||
|
||
if math_sign == False: | ||
return 'Enter valid expression' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Опять же по идее должны делать raise
. Вы ж даже в типах указали float, а получается, что может вернуться еще и строка :)
Но пока что ладно ))
return a + b | ||
|
||
|
||
list_of_operators = ['/', '*', '-', '+'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Если вы пишете это за пределами функции, тогда тут это константа :)
Такое нужно прописными буквами называть: list_of_operators
--> LIST_OF_OPERATORS
return multiplication(num1, num2) | ||
|
||
except ValueError: | ||
return 'Enter valid expression' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Та же история с raise
[ZoneTransfer] | ||
ZoneId=3 | ||
HostUrl=https://web.telegram.org/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Это зачем тут? :)
Если это не нужно, то, на будущее, такое лучше добавлять в .gitignore
и смотреть, что добавляете через git add
))
@@ -0,0 +1,42 @@ | |||
# Calculator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
В следующий раз сразу меняй содержимое README.md
:)
Calculator app from team